Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[TASK] Use PHPUnit 9.x with PHP 7.3+ #930

Merged
merged 7 commits into from
Nov 22, 2020
Merged

[TASK] Use PHPUnit 9.x with PHP 7.3+ #930

merged 7 commits into from
Nov 22, 2020

Conversation

JakeQZ
Copy link
Contributor

@JakeQZ JakeQZ commented Nov 21, 2020 •
edited
Loading

This is required to be able to test against PHP 8. However, PHPUnit 9.x
requires PHP 7.3, so a different version of PHPUnit is required for different
testing environments.

The main change here is a step in the GitHub Action to conditionally update
PHPUnit via PHIVE for PHP >=7.3. PHIVE does not yet have the ability for
conditional installs (see
phar-io/phive#295 (comment)) so the
script must check the PHP version before running the update.

PHIVE has also been added to the tools (self-referencing) as it is not available
by default to GitHub Actions.

Note: There are warnings from PHPUnit 9.x about use of deprecated assert
methods (which will be removed in PHPUnit 10.x). However, these don't cause the
tests to fail, and the replacement methods are not available in PHPUnit 7.x
which is still required to test against PHP 7.1 and 7.2.

Part of #925/#926.

@JakeQZ JakeQZ added this to the 5.0.0 milestone Nov 21, 2020
@JakeQZ JakeQZ requested a review from oliverklee November 21, 2020 00:58
@JakeQZ JakeQZ self-assigned this Nov 21, 2020
@JakeQZ JakeQZ force-pushed the compat/phpunit-9 branch 2 times, most recently from 1b8269b to 1eb5f29 Compare November 21, 2020 01:10
@JakeQZ JakeQZ mentioned this pull request Nov 21, 2020
Closed
oliverklee
oliverklee requested changes Nov 21, 2020
View reviewed changes
composer.json Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
oliverklee
oliverklee reviewed Nov 21, 2020
View reviewed changes
composer.json Outdated Show resolved Hide resolved
@JakeQZ
Copy link
Contributor Author

JakeQZ commented Nov 21, 2020

Managed to add --no-progress (it seems the options must come first on the command line), but it seems --trust-gpg-keys is not supported in PHIVE version 0.13.5 (I get 'unknown option' whether it's first or last on the command line)?

Where is the documentation for PHIVE?

@oliverklee
Copy link
Contributor

Where is the documentation for PHIVE?

It's built-in: #930 (comment)

oliverklee
oliverklee requested changes Nov 21, 2020
View reviewed changes
Copy link
Contributor

@oliverklee oliverklee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please let's use the latest PHIVE version and explicitly allow Sebastian's GPG key.

@JakeQZ
Copy link
Contributor Author

JakeQZ commented Nov 22, 2020 •
edited
Loading

explicitly allow Sebastian's GPG key.

This does not appear to be possible. The --trust-gpg-keys option is only available with the install command, not with the update command. With the install command, it does not seem possible to specify that the PHAR file be copied with a .phar extension (it is copied with no file extension, as simply e.g. phpunit), nor any version constraints - any pre-existing entry in phive.xml is simply ignored and overwritten.

Some proper documentation for PHIVE would be really helpful.

@oliverklee
Copy link
Contributor

Okay, then it seems we need to keep this workaround for trusting the GPG key.

Please let's still use the latest PHIVE version.

@JakeQZ
Copy link
Contributor Author

JakeQZ commented Nov 22, 2020

I've added phar-io/phive#296.

@JakeQZ
Copy link
Contributor Author

JakeQZ commented Nov 22, 2020

Please let's still use the latest PHIVE version.

Done.

@oliverklee
Copy link
Contributor

I think #906 has introduced some changes that are not compatible with PHPUnit 9.

@oliverklee
Copy link
Contributor

PHPUnit's Constraint parent class does not have a constructor in PHPUnit 9.x anymore. So maybe we can copy the old version's constructor to our CssConstraint class:

    public function __construct()
    {
        $this->exporter = new Exporter();
    }

(And create an issue that we should rework this once we're PHPUnit-9-only.)

@JakeQZ
[TASK] Use PHPUnit 9.x with PHP 7.3+
3fbc41d 
This is required to be able to test against PHP 8.  However, PHPUnit 9.x
requires PHP 7.3, so a different version of PHPUnit is required for different
testing environments.

The main change here is a step in the GitHub Action to conditionally update
PHPUnit via PHIVE for PHP >=7.3.  PHIVE does not yet have the ability for
conditional installs (see
phar-io/phive#295 (comment)) so the
script must check the PHP version before running the update.

PHIVE has also been added to the tools (self-referencing) as it is not available
by default to GitHub Actions.

Note: There are warnings from PHPUnit 9.x about use of deprecated `assert`
methods (which will be removed in PHPUnit 10.x).  However, these don't cause the
tests to fail, and the replacement methods are not available in PHPUnit 7.x
which is still required to test against PHP 7.1 and 7.2.

Part of #925/#926.
@JakeQZ
Use --trust-gpg-keys and --no-progress on PHIVE command line.
0bb6617
@JakeQZ
Remove --trust-gpg-keys PHIVE command line option.
6eb372e
@JakeQZ
Move PHIVE command line option before command.
23efe2f
@JakeQZ
Update PHIVE to version 0.14.4.
999c536
@JakeQZ
Copy link
Contributor Author

JakeQZ commented Nov 22, 2020

(And create an issue that we should rework this once we're PHPUnit-9-only.)

I've added a constructor which will conditionally call the parent constructor only if it exists. This should cover all bases, including if some future version of PHPUnit reintroduces a constructor. So I don't think any future rework would be required.

I can't help thinking that PHP should provide an implicit default constructor, as C++ does, to avoid this type of issue.

@JakeQZ
Change constructor access level to public for PHP 7.1 compatibility.
8a52bb5 
Only since PHP 7.2 can the access level in a child class be made more
restrictive.  See [PHP bug #61970](https://bugs.php.net/bug.php?id=61970).
oliverklee
oliverklee approved these changes Nov 22, 2020
View reviewed changes
Copy link
Contributor

@oliverklee oliverklee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<3

@oliverklee oliverklee merged commit 6b679ac into master Nov 22, 2020
@oliverklee oliverklee deleted the compat/phpunit-9 branch November 22, 2020 12:16
@JakeQZ
Copy link
Contributor Author

JakeQZ commented Nov 25, 2020

There are some suggestions for how to handle PHIVE in relation to this in the responses to phar-io/phive#296. I don't know if this is something we want to address in the short term; it will become a non-issue when we drop PHP 7.2 support.

@JakeQZ JakeQZ mentioned this pull request Apr 10, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliverklee oliverklee oliverklee approved these changes

Assignees

@JakeQZ JakeQZ

Labels
enhancement
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@JakeQZ @oliverklee